Codecov Report

Merging #752 (5022573) into master (0675ebb) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
- Coverage   92.72%   92.71%   -0.01%     
==========================================
  Files          68       68              
  Lines        3668     3666       -2     
==========================================
- Hits         3401     3399       -2     
  Misses        267      267              

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0675ebb...5022573. Read the comment docs.

Might be tricky, but is there any way we can add a middleware test that checks for the original bug? It's still not clear to me why localhost:5000/v1 would cause a VersionNotSupported error before these changes

Yeah. I've been looking into this for a while now today, and it all comes back to the issue of not being able to test non-JSON endpoints, i.e., we cannot properly test the landing page endpoints using Starlette's TestClient (see the related issue encode/starlette#472).

So I am in the process of trying to simply turn to using a different TestClient, so far I have in my sights httpx and async-asgi-testclient. The former is suggested in the FastAPI documentation when wanting to test a fully asynchronous server (the code being built using asynchronous functions). The latter I found based on a related issue in FastAPI (fastapi/fastapi#806).

As a note, I also found we're not using a landing page for the index meta-database.

Gah. I've come to the conclusion this will require a proper update of the way we test at the moment. We would need to move to testing with an asynchronous client, completely, which would turn many of the actual test functions into async functions themselves.

We should make this part of turning it all asynchronous, in the future, I think - keeping specific functions and capabilities synchronous if the backend driver does not support it?

There is something I don't really understand here though. And that's that the core issue comes from an assert statement in the BaseHTTPMiddelware class' call_next() method. This is not a class that's specific to the Starlette TestClient, and so the issue should also arise under "normal" conditions, but it doesn't. So it might the way we're using the TestClient that's the issue? But I'm not sure. I guess this is already explained in the Starlette issue though.

I checked the new test implemented in 5022573 locally after rebasing and dropping the first two commits and it indeed failed. So it successfully tests that the CheckWronglyVersionedBaseUrls middleware doesn't screw up reaching the landing page for versioned base URLs.
Of course this is a very specific test and it would be better to test that we can reach the specific landing pages instead. But as you can see from my comments above, it doesn't seem to be as easy as all that... unfortunately.

Thanks for your hard work @CasperWA, sounds pretty tricky.

👍

A simple option might be to configure the landing page (so that it can be disabled in the case of the test client), i.e. it could serve a JSON landing page or otherwise. I can't say I fully understand the technical details of these issues so I will leave it up to you to decide the best course of action.

As hinted above, I'd prefer to move to a fully asynchronous test suite, where possible. Use a framework-agnostic test client like either of the packages supply mentioned above and go through both the code base and the tests to make everything asynchronous that can be asynchronous.